Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[pigeon] fix named parameters in flutter api #5663

Closed
wants to merge 7 commits into from
Closed

[pigeon] fix named parameters in flutter api #5663

wants to merge 7 commits into from

Conversation

schultek
Copy link

@schultek schultek commented Dec 13, 2023 •
edited
Loading

Fixes flutter/flutter#140045

  • Correctly generates code for flutter api methods with named parameters
  • Correctly generates test code for host api methods with named parameters

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla Google CLA
Copy link

google-cla bot commented Dec 13, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

schultek
schultek commented Dec 13, 2023
View reviewed changes
packages/pigeon/lib/ast.dart
@@ -277,7 +277,7 @@ class Parameter extends NamedType {
bool? isPositional,
bool? isRequired,
super.documentationComments,
}) : isNamed = isNamed ?? true,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes existing tests that don't specify this but still assume it's false.

Since this wasn't used anywhere besides toString, I don't consider this a breaking change.

@tarrinneal
Copy link
Contributor

Can you add an integration test for this as well? If you need guidance on this, let me know.

This will also need versioning (changelog, pubspec, and generator_tools.dart)

@schultek
Copy link
Author

@tarrinneal Thanks for reviewing so far.

I updated the versioning stuff and started with the integration tests, but I need your help on this:

I added some appropriate methods here: packages/pigeon/pigeons/core_tests.dart
But generating and updating all the resulting native files is a lot and I neither have the knowledge nor tooling installed to handle all the platforms. Can you help by generating the files and updating the native implementations? Then I can continue with the dart parts.

@tarrinneal
Copy link
Contributor

There's actually another bug that's surfaced from this (I actually intended for this feature to only be on hostApi's). I'm going to patch into this pr, unless you prefer I don't.

@tarrinneal tarrinneal mentioned this pull request Dec 15, 2023
Merged
11 tasks
@tarrinneal
Copy link
Contributor

closing in favor of #5689

@schultek Thank you for putting this pr together. Unfortunately for now, we are going to be removing this feature as it was never intended to be added, and needs more work than just adding these fixes.

@tarrinneal tarrinneal closed this Dec 15, 2023
auto-submit bot pushed a commit that referenced this pull request Dec 16, 2023
@tarrinneal
Remove flutter api named and optional parameters (#5689)
d7dee79 
fixes flutter/flutter#140045
replaces #5663 as no longer needed.
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@tarrinneal @arc-yong
Remove flutter api named and optional parameters (flutter#5689)
48e16a4 
fixes flutter/flutter#140045
replaces flutter#5663 as no longer needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarrinneal tarrinneal Awaiting requested review from tarrinneal tarrinneal is a code owner

@stuartmorgan stuartmorgan Awaiting requested review from stuartmorgan

Assignees
No one assigned
Labels
p: pigeon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[pigeon] Fix named arguments in methods.
2 participants
@schultek @tarrinneal